Skip to content

gh-118965: Share NotImplemented in subinterpreters - #118966

Open
sobolevn wants to merge 5 commits into
python:mainfrom
sobolevn:issue-118965
Open

gh-118965: Share NotImplemented in subinterpreters#118966
sobolevn wants to merge 5 commits into
python:mainfrom
sobolevn:issue-118965

Conversation

@sobolevn

@sobolevn sobolevn commented May 12, 2024 •
edited
Loading

Copy link
Copy Markdown
Member

I still use Py_NewRef despite the fact that NotImplemented is immortal:

static void
notimplemented_dealloc(PyObject *notimplemented)
{
    /* This should never get called, but we also don't want to SEGV if
     * we accidentally decref NotImplemented out of existence. Instead,
     * since Notimplemented is an immortal object, re-set the reference count.
     */
    _Py_SetImmortal(notimplemented);
}

@bedevere-app bedevere-app Bot mentioned this pull request May 12, 2024
Open
@nineteendo

nineteendo commented May 12, 2024 •
edited
Loading

Copy link
Copy Markdown
Contributor

cpython/Lib/test/test_interpreters/test_api.py

Lines 1128 to 1130 in 176b42a

not_shareables = [
# singletons
NotImplemented,

Should be moved to

cpython/Lib/test/test_interpreters/test_api.py

Lines 1099 to 1101 in 176b42a

shareables = [
# singletons
None,

By the way, is there a reason ... is not shareable?

@sobolevn

sobolevn commented May 12, 2024

Copy link
Copy Markdown
Member Author

@nineteendo thanks.

By the way, is there a reason ... is not shareable?

I don't think so. I plan to address it in the next PR.

@ericsnowcurrently

ericsnowcurrently commented May 13, 2024

Copy link
Copy Markdown
Member

FWIW, there's no urgency here. The other singletons have a clear value in being shareable. However, NotImplemented and Ellipsis do not. There's nothing wrong with making them shareable. However, there isn't much clear advantage either, AFAIK.

erlend-aasland
erlend-aasland reviewed May 18, 2024
View reviewed changes
Comment thread Python/crossinterp_data_lookup.h
Comment on lines +582 to +587
// NotImplemented
if (_xidregistry_add_type(xidregistry,
(PyTypeObject *)PyObject_Type(Py_NotImplemented),
_notimplemented_shared) != 0) {
Py_FatalError("could not register NotImplemented for cross-interpreter sharing");
}

@erlend-aasland erlend-aasland May 18, 2024

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we generalize this with a macro? (Future refactoring)

@ZeroIntensity ZeroIntensity Feb 9, 2025

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think so. Last I heard, Eric's plan was to eventually get a dedicated type slot for crossinterpreter registry things, so this will (hopefully) be obsolete sooner than later.

ZeroIntensity
ZeroIntensity reviewed Feb 9, 2025
View reviewed changes

@ZeroIntensity ZeroIntensity left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Trying to work through the subinterpreter backlog a bit :)

Comment thread Python/crossinterp_data_lookup.h
Comment on lines +582 to +587
// NotImplemented
if (_xidregistry_add_type(xidregistry,
(PyTypeObject *)PyObject_Type(Py_NotImplemented),
_notimplemented_shared) != 0) {
Py_FatalError("could not register NotImplemented for cross-interpreter sharing");
}

@ZeroIntensity ZeroIntensity Feb 9, 2025

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think so. Last I heard, Eric's plan was to eventually get a dedicated type slot for crossinterpreter registry things, so this will (hopefully) be obsolete sooner than later.

Comment thread Python/crossinterp_data_lookup.h Outdated
Comment thread Misc/NEWS.d/next/Core and Builtins/2024-05-12-15-19-40.gh-issue-118965.Lr3qAz.rst Outdated
@ZeroIntensity

ZeroIntensity commented Feb 9, 2025 •
edited
Loading

Copy link
Copy Markdown
Member

Merging from main seemed to blow up CI. I think something changed with XI registries after this was created.

@github-actions

github-actions Bot commented Apr 16, 2026

Copy link
Copy Markdown

This PR is stale because it has been open for 30 days with no activity.

@github-actions github-actions Bot added the stale Stale PR or inactive for long period of time. label Apr 16, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@erlend-aasland erlend-aasland erlend-aasland left review comments
@ZeroIntensity ZeroIntensity ZeroIntensity left review comments
@ericsnowcurrently ericsnowcurrently Awaiting requested review from ericsnowcurrently ericsnowcurrently is a code owner

Assignees

No one assigned

Labels

awaiting core review stale Stale PR or inactive for long period of time.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

@sobolevn @nineteendo @ericsnowcurrently @ZeroIntensity @erlend-aasland